Update jdk-javac-plugin to contain nested annotations #1432
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
WalkthroughAdds detection, modeling, and serialization of nested JSpecify nullability annotations: introduces a public NestedAnnotationInfo record (with TypePathEntry and enums); implements CreateNestedAnnotationInfoVisitor to collect nested Nullable/NonNull annotations in class type arguments, array elements, and wildcard bounds; extends MethodInfo to include a Map<Integer, Set> holding return and per-parameter nested annotations and updates NullnessAnnotationSerializer to populate that map; adds Gson ImmutableList adapters for JSON parsing in tests and astubx-generator; and adds a Guava implementation dependency to the plugin build. Possibly related issues
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used🧠 Learnings (8)📓 Common learnings📚 Learning: 2025-08-14T18:50:06.159ZApplied to files:
📚 Learning: 2025-09-24T23:14:37.777ZApplied to files:
📚 Learning: 2025-08-28T04:54:20.953ZApplied to files:
📚 Learning: 2025-10-29T23:56:18.236ZApplied to files:
📚 Learning: 2025-08-29T18:41:43.584ZApplied to files:
📚 Learning: 2025-09-03T19:40:11.118ZApplied to files:
📚 Learning: 2025-12-31T23:59:20.009ZApplied to files:
🔇 Additional comments (8)
✏️ Tip: You can disable this entire section by setting Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In
@jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/CreateNestedAnnotationInfoVisitor.java:
- Around line 116-120: The visitType method in CreateNestedAnnotationInfoVisitor
currently returns null; change it to return an empty set for consistency with
other visit methods (e.g., return Collections.emptySet() or an immutable empty
Set) so callers don't need to special-case null; update the import if needed and
keep the @Nullable signature intact while returning Collections.emptySet()
instead of null.
- Around line 23-24: In CreateNestedAnnotationInfoVisitor, change the instance
fields "path" (ArrayDeque<TypePathEntry>) and "nestedAnnotationInfoList"
(Set<NestedAnnotationInfo>) from package-private to private to encapsulate
state; update any external accesses to use new accessors or move usage into the
class (add private getters only if needed) and ensure constructor/visitor
methods inside CreateNestedAnnotationInfoVisitor still reference these fields
directly.
In
@jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.java:
- Around line 180-192: The current guard "if (!methodHasAnnotations)" prevents
scanning parameters when the return type already set methodHasAnnotations, so
nested parameter annotations are skipped; inside the method handling for
Symbol.MethodSymbol mSym, always iterate over mSym.getParameters() and run
hasJSpecifyAnnotationDeep and vSym.asType().accept(new
CreateNestedAnnotationInfoVisitor(), null) for each parameter, setting
methodHasAnnotations when any annotation is found and populating
nestedAnnotationsMap.put(idx, nested) when nested is non-null/non-empty; remove
or move the early guard so parameter scanning runs unconditionally while
preserving the existing logic that sets methodHasAnnotations and fills
nestedAnnotationsMap.
- Around line 159-162: Remove the dead/commented code and the no-op null check:
delete the commented ImmutableSetMultimap.Builder line and the empty conditional
that checks nestedAnnotationsMap for null; keep the valid initialization
Map<Integer, Set<NestedAnnotationInfo>> nestedAnnotationsMap = new HashMap<>();
and ensure no other references depend on the removed lines in
NullnessAnnotationSerializer (search for nestedAnnotationsBuilder or the no-op
if block to confirm).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
jdk-javac-plugin/build.gradlejdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/CreateNestedAnnotationInfoVisitor.javajdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.javajdk-javac-plugin/src/test/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializerTest.java
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: haewiful
Repo: uber/NullAway PR: 1243
File: jdk-annotations/astubx-generator-cli/src/main/java/com/uber/nullaway/jdkannotations/AstubxGeneratorCLI.java:97-106
Timestamp: 2025-09-24T23:14:37.777Z
Learning: In the jdk-javac-plugin NullnessAnnotationSerializer, the plugin only creates one JSON file per compilation, so processing only the first JSON file is intentional behavior rather than a bug.
Learnt from: msridhar
Repo: uber/NullAway PR: 1248
File: nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java:847-857
Timestamp: 2025-08-28T04:54:20.953Z
Learning: In NullAway's GenericsChecks.java, NewClassTree support for explicit type argument substitution requires more extensive changes beyond just modifying the conditional in compareGenericTypeParameterNullabilityForCall. The maintainers prefer to handle NewClassTree support in a separate follow-up rather than expanding the scope of PRs focused on specific issues like super constructor calls.
Learnt from: msridhar
Repo: uber/NullAway PR: 1316
File: jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.java:261-293
Timestamp: 2025-10-29T23:56:18.236Z
Learning: In NullAway's jdk-javac-plugin NullnessAnnotationSerializer, type variable bounds with annotations (e.g., `T extends Nullable Object`) are checked at their declaration sites by the typeParamHasAnnotation method for both class-level and method-level type parameters. The hasJSpecifyAnnotationDeep method is designed to check type uses (return types, parameters, etc.) and does not need a TYPEVAR case because type variable declaration bounds are already handled separately.
Learnt from: msridhar
Repo: uber/NullAway PR: 1245
File: guava-recent-unit-tests/src/test/java/com/uber/nullaway/guava/NullAwayGuavaParametricNullnessTests.java:101-102
Timestamp: 2025-08-14T18:50:06.159Z
Learning: In NullAway JSpecify tests, when JDK version requirements exist due to bytecode annotation reading capabilities, prefer failing tests over skipping them on unsupported versions to ensure CI catches regressions and enforces proper JDK version usage for developers.
📚 Learning: 2025-10-29T23:56:18.236Z
Learnt from: msridhar
Repo: uber/NullAway PR: 1316
File: jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.java:261-293
Timestamp: 2025-10-29T23:56:18.236Z
Learning: In NullAway's jdk-javac-plugin NullnessAnnotationSerializer, type variable bounds with annotations (e.g., `T extends Nullable Object`) are checked at their declaration sites by the typeParamHasAnnotation method for both class-level and method-level type parameters. The hasJSpecifyAnnotationDeep method is designed to check type uses (return types, parameters, etc.) and does not need a TYPEVAR case because type variable declaration bounds are already handled separately.
Applied to files:
jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.javajdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/CreateNestedAnnotationInfoVisitor.javajdk-javac-plugin/src/test/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializerTest.java
📚 Learning: 2025-08-28T04:54:20.953Z
Learnt from: msridhar
Repo: uber/NullAway PR: 1248
File: nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java:847-857
Timestamp: 2025-08-28T04:54:20.953Z
Learning: In NullAway's GenericsChecks.java, NewClassTree support for explicit type argument substitution requires more extensive changes beyond just modifying the conditional in compareGenericTypeParameterNullabilityForCall. The maintainers prefer to handle NewClassTree support in a separate follow-up rather than expanding the scope of PRs focused on specific issues like super constructor calls.
Applied to files:
jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.javajdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/CreateNestedAnnotationInfoVisitor.javajdk-javac-plugin/src/test/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializerTest.java
📚 Learning: 2025-08-14T18:50:06.159Z
Learnt from: msridhar
Repo: uber/NullAway PR: 1245
File: guava-recent-unit-tests/src/test/java/com/uber/nullaway/guava/NullAwayGuavaParametricNullnessTests.java:101-102
Timestamp: 2025-08-14T18:50:06.159Z
Learning: In NullAway JSpecify tests, when JDK version requirements exist due to bytecode annotation reading capabilities, prefer failing tests over skipping them on unsupported versions to ensure CI catches regressions and enforces proper JDK version usage for developers.
Applied to files:
jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.javajdk-javac-plugin/build.gradlejdk-javac-plugin/src/test/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializerTest.java
📚 Learning: 2025-09-24T23:14:37.777Z
Learnt from: haewiful
Repo: uber/NullAway PR: 1243
File: jdk-annotations/astubx-generator-cli/src/main/java/com/uber/nullaway/jdkannotations/AstubxGeneratorCLI.java:97-106
Timestamp: 2025-09-24T23:14:37.777Z
Learning: In the jdk-javac-plugin NullnessAnnotationSerializer, the plugin only creates one JSON file per compilation, so processing only the first JSON file is intentional behavior rather than a bug.
Applied to files:
jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.javajdk-javac-plugin/src/test/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializerTest.java
📚 Learning: 2025-09-03T19:40:11.118Z
Learnt from: msridhar
Repo: uber/NullAway PR: 1276
File: nullaway/src/main/java/com/uber/nullaway/generics/TypeMetadataBuilder.java:235-241
Timestamp: 2025-09-03T19:40:11.118Z
Learning: In JDK 21+, ListBuffer can be passed directly to TypeMetadata.Annotations constructor without needing conversion to List via toList() method. The APIs were updated to make this compatible.
Applied to files:
jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.java
📚 Learning: 2025-12-31T23:59:20.009Z
Learnt from: msridhar
Repo: uber/NullAway PR: 1421
File: nullaway/src/main/java/com/uber/nullaway/NullAway.java:339-340
Timestamp: 2025-12-31T23:59:20.009Z
Learning: In Error Prone's ASTHelpers class, the getSymbol(MethodInvocationTree) overload is guaranteed to return a non-null Symbol.MethodSymbol. This differs from other overloads of getSymbol() that may return null. Therefore, no null check is required after calling ASTHelpers.getSymbol() with a MethodInvocationTree parameter.
Applied to files:
jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.java
📚 Learning: 2025-11-25T22:43:06.446Z
Learnt from: CR
Repo: uber/NullAway PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T22:43:06.446Z
Learning: Run only the tests for the main NullAway module using `./gradlew :nullaway:test` unless specifically asked to run tests in a different module
Applied to files:
jdk-javac-plugin/build.gradle
🧬 Code graph analysis (1)
jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/CreateNestedAnnotationInfoVisitor.java (1)
jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.java (1)
NullMarked(43-333)
🔇 Additional comments (5)
jdk-javac-plugin/src/test/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializerTest.java (1)
404-512: Good test coverage for nested annotations.The
nestedAnnotationsForMethods()test provides thorough coverage of:
- Nested annotations in type arguments (
Class<@NonNull E>)- Return type nested annotations (
List<@Nullable String>)- Array annotations (
String @Nullable [],List<@NonNull String>[])- Wildcard bounds (
? extends @Nullable String)This validates the core functionality of the
CreateNestedAnnotationInfoVisitor.jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/CreateNestedAnnotationInfoVisitor.java (3)
56-73: Verify array annotation handling semantics.For
String @Nullable [], the annotation is on the array type itself. The code adds anARRAY_ELEMENTpath entry and then checks annotations on theArrayTypet, not ont.elemtype.Per Java's type annotation semantics,
String @Nullable []has the annotation on the array dimension. However, the path entryARRAY_ELEMENTtypically means "navigate into the element type." Please verify this matches the expected TypePath semantics fromjava.lang.reflect.AnnotatedTypeor JVM bytecode type paths.
31-54: LGTM: visitClassType implementation is correct.The method properly:
- Iterates through type arguments with correct indexing
- Maintains the path stack with push/pop semantics
- Checks for both
@Nullableand@NonNullannotations- Creates immutable path snapshots before recording
- Recursively visits nested type structures
75-114: LGTM: Wildcard handling correctly distinguishes upper and lower bounds.The implementation correctly uses:
WILDCARD_BOUND, 0for extends bounds (? extends T)WILDCARD_BOUND, 1for super bounds (? super T)This matches the JVM TypePath encoding where 0 indicates an upper bound and 1 indicates a lower bound for wildcards.
jdk-javac-plugin/build.gradle (1)
60-61: Dependency structure is safe—no circular dependency risk.Adding
project(':nullaway')as an implementation dependency introduces coupling betweenjdk-javac-pluginand the mainnullawaymodule, which is necessary to shareNestedAnnotationInfo. Thenullawaymodule has no dependency onjdk-javac-plugin, so there is no circular dependency concern.
...ac-plugin/src/main/java/com/uber/nullaway/javacplugin/CreateNestedAnnotationInfoVisitor.java
Outdated
Show resolved
Hide resolved
...ac-plugin/src/main/java/com/uber/nullaway/javacplugin/CreateNestedAnnotationInfoVisitor.java
Show resolved
Hide resolved
jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.java
Outdated
Show resolved
Hide resolved
jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.java
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
jdk-annotations/astubx-generator/src/main/java/com/uber/nullaway/jdkannotations/AstubxGenerator.java
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: msridhar
Repo: uber/NullAway PR: 1248
File: nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java:847-857
Timestamp: 2025-08-28T04:54:20.953Z
Learning: In NullAway's GenericsChecks.java, NewClassTree support for explicit type argument substitution requires more extensive changes beyond just modifying the conditional in compareGenericTypeParameterNullabilityForCall. The maintainers prefer to handle NewClassTree support in a separate follow-up rather than expanding the scope of PRs focused on specific issues like super constructor calls.
Learnt from: haewiful
Repo: uber/NullAway PR: 1243
File: jdk-annotations/astubx-generator-cli/src/main/java/com/uber/nullaway/jdkannotations/AstubxGeneratorCLI.java:97-106
Timestamp: 2025-09-24T23:14:37.777Z
Learning: In the jdk-javac-plugin NullnessAnnotationSerializer, the plugin only creates one JSON file per compilation, so processing only the first JSON file is intentional behavior rather than a bug.
Learnt from: msridhar
Repo: uber/NullAway PR: 1316
File: jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.java:261-293
Timestamp: 2025-10-29T23:56:18.236Z
Learning: In NullAway's jdk-javac-plugin NullnessAnnotationSerializer, type variable bounds with annotations (e.g., `T extends Nullable Object`) are checked at their declaration sites by the typeParamHasAnnotation method for both class-level and method-level type parameters. The hasJSpecifyAnnotationDeep method is designed to check type uses (return types, parameters, etc.) and does not need a TYPEVAR case because type variable declaration bounds are already handled separately.
Learnt from: msridhar
Repo: uber/NullAway PR: 1245
File: guava-recent-unit-tests/src/test/java/com/uber/nullaway/guava/NullAwayGuavaParametricNullnessTests.java:101-102
Timestamp: 2025-08-14T18:50:06.159Z
Learning: In NullAway JSpecify tests, when JDK version requirements exist due to bytecode annotation reading capabilities, prefer failing tests over skipping them on unsupported versions to ensure CI catches regressions and enforces proper JDK version usage for developers.
Learnt from: msridhar
Repo: uber/NullAway PR: 1259
File: jdk-recent-unit-tests/src/test/java/com/uber/nullaway/jdk17/SwitchTests.java:318-321
Timestamp: 2025-08-29T18:41:43.584Z
Learning: Classes annotated with NullMarked are analyzed by NullAway even if they are not in packages specified by the AnnotatedPackages configuration. The NullMarked annotation guarantees NullAway analysis.
📚 Learning: 2025-09-03T19:40:11.118Z
Learnt from: msridhar
Repo: uber/NullAway PR: 1276
File: nullaway/src/main/java/com/uber/nullaway/generics/TypeMetadataBuilder.java:235-241
Timestamp: 2025-09-03T19:40:11.118Z
Learning: In JDK 21+, ListBuffer can be passed directly to TypeMetadata.Annotations constructor without needing conversion to List via toList() method. The APIs were updated to make this compatible.
Applied to files:
jdk-annotations/astubx-generator/src/main/java/com/uber/nullaway/jdkannotations/AstubxGenerator.java
📚 Learning: 2025-09-24T23:14:37.777Z
Learnt from: haewiful
Repo: uber/NullAway PR: 1243
File: jdk-annotations/astubx-generator-cli/src/main/java/com/uber/nullaway/jdkannotations/AstubxGeneratorCLI.java:97-106
Timestamp: 2025-09-24T23:14:37.777Z
Learning: In the jdk-javac-plugin NullnessAnnotationSerializer, the plugin only creates one JSON file per compilation, so processing only the first JSON file is intentional behavior rather than a bug.
Applied to files:
jdk-annotations/astubx-generator/src/main/java/com/uber/nullaway/jdkannotations/AstubxGenerator.java
🔇 Additional comments (1)
jdk-annotations/astubx-generator/src/main/java/com/uber/nullaway/jdkannotations/AstubxGenerator.java (1)
3-3: LGTM!The new imports are necessary to support the custom Gson type adapter for
ImmutableListdeserialization.Also applies to: 7-8, 19-19
...tations/astubx-generator/src/main/java/com/uber/nullaway/jdkannotations/AstubxGenerator.java
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
@jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.java:
- Around line 161-170: Return-type nested annotation collection is done
unconditionally while parameter nested collection is gated by
hasJSpecifyAnnotationDeep; make them consistent by always invoking the
CreateNestedAnnotationInfoVisitor for parameter types (like you do for
mSym.getReturnType()) and inserting any non-empty Set<NestedAnnotationInfo> into
nestedAnnotationsMap keyed by the parameter index. Keep the existing
hasJSpecifyAnnotationDeep checks solely to set methodHasAnnotations (do not use
it to skip running paramType.accept), and use the same pattern/symbols used for
the return type: CreateNestedAnnotationInfoVisitor, nestedAnnotationsMap,
methodHasAnnotations, and the parameter type accessors (the param symbol/type
used where mSym.getReturnType() is used).
In
@jdk-javac-plugin/src/test/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializerTest.java:
- Around line 519-535: The deserializer assigned to
JsonDeserializer<ImmutableList<?>> (immutableListAdapter) unsafely casts typeOfT
to ParameterizedType and assumes json.isJsonArray() is true; update it to first
check that typeOfT is an instance of ParameterizedType before extracting actual
type arguments (handle raw ImmutableList by returning empty list or throwing a
clear JsonParseException), and defensively handle json being null or JsonNull
and non-array JsonElements by either returning an empty ImmutableList or
throwing a descriptive JsonParseException; ensure you still use
context.deserialize(element, contentClass) when contentClass is available.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.javajdk-javac-plugin/src/test/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializerTest.java
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: msridhar
Repo: uber/NullAway PR: 1248
File: nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java:847-857
Timestamp: 2025-08-28T04:54:20.953Z
Learning: In NullAway's GenericsChecks.java, NewClassTree support for explicit type argument substitution requires more extensive changes beyond just modifying the conditional in compareGenericTypeParameterNullabilityForCall. The maintainers prefer to handle NewClassTree support in a separate follow-up rather than expanding the scope of PRs focused on specific issues like super constructor calls.
Learnt from: msridhar
Repo: uber/NullAway PR: 1316
File: jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.java:261-293
Timestamp: 2025-10-29T23:56:18.236Z
Learning: In NullAway's jdk-javac-plugin NullnessAnnotationSerializer, type variable bounds with annotations (e.g., `T extends Nullable Object`) are checked at their declaration sites by the typeParamHasAnnotation method for both class-level and method-level type parameters. The hasJSpecifyAnnotationDeep method is designed to check type uses (return types, parameters, etc.) and does not need a TYPEVAR case because type variable declaration bounds are already handled separately.
Learnt from: msridhar
Repo: uber/NullAway PR: 1245
File: guava-recent-unit-tests/src/test/java/com/uber/nullaway/guava/NullAwayGuavaParametricNullnessTests.java:101-102
Timestamp: 2025-08-14T18:50:06.159Z
Learning: In NullAway JSpecify tests, when JDK version requirements exist due to bytecode annotation reading capabilities, prefer failing tests over skipping them on unsupported versions to ensure CI catches regressions and enforces proper JDK version usage for developers.
Learnt from: msridhar
Repo: uber/NullAway PR: 1259
File: jdk-recent-unit-tests/src/test/java/com/uber/nullaway/jdk17/SwitchTests.java:318-321
Timestamp: 2025-08-29T18:41:43.584Z
Learning: Classes annotated with NullMarked are analyzed by NullAway even if they are not in packages specified by the AnnotatedPackages configuration. The NullMarked annotation guarantees NullAway analysis.
Learnt from: haewiful
Repo: uber/NullAway PR: 1243
File: jdk-annotations/astubx-generator-cli/src/main/java/com/uber/nullaway/jdkannotations/AstubxGeneratorCLI.java:97-106
Timestamp: 2025-09-24T23:14:37.777Z
Learning: In the jdk-javac-plugin NullnessAnnotationSerializer, the plugin only creates one JSON file per compilation, so processing only the first JSON file is intentional behavior rather than a bug.
📚 Learning: 2025-09-24T23:14:37.777Z
Learnt from: haewiful
Repo: uber/NullAway PR: 1243
File: jdk-annotations/astubx-generator-cli/src/main/java/com/uber/nullaway/jdkannotations/AstubxGeneratorCLI.java:97-106
Timestamp: 2025-09-24T23:14:37.777Z
Learning: In the jdk-javac-plugin NullnessAnnotationSerializer, the plugin only creates one JSON file per compilation, so processing only the first JSON file is intentional behavior rather than a bug.
Applied to files:
jdk-javac-plugin/src/test/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializerTest.javajdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.java
📚 Learning: 2025-08-14T18:50:06.159Z
Learnt from: msridhar
Repo: uber/NullAway PR: 1245
File: guava-recent-unit-tests/src/test/java/com/uber/nullaway/guava/NullAwayGuavaParametricNullnessTests.java:101-102
Timestamp: 2025-08-14T18:50:06.159Z
Learning: In NullAway JSpecify tests, when JDK version requirements exist due to bytecode annotation reading capabilities, prefer failing tests over skipping them on unsupported versions to ensure CI catches regressions and enforces proper JDK version usage for developers.
Applied to files:
jdk-javac-plugin/src/test/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializerTest.javajdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.java
📚 Learning: 2025-08-28T04:54:20.953Z
Learnt from: msridhar
Repo: uber/NullAway PR: 1248
File: nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java:847-857
Timestamp: 2025-08-28T04:54:20.953Z
Learning: In NullAway's GenericsChecks.java, NewClassTree support for explicit type argument substitution requires more extensive changes beyond just modifying the conditional in compareGenericTypeParameterNullabilityForCall. The maintainers prefer to handle NewClassTree support in a separate follow-up rather than expanding the scope of PRs focused on specific issues like super constructor calls.
Applied to files:
jdk-javac-plugin/src/test/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializerTest.javajdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.java
📚 Learning: 2025-10-29T23:56:18.236Z
Learnt from: msridhar
Repo: uber/NullAway PR: 1316
File: jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.java:261-293
Timestamp: 2025-10-29T23:56:18.236Z
Learning: In NullAway's jdk-javac-plugin NullnessAnnotationSerializer, type variable bounds with annotations (e.g., `T extends Nullable Object`) are checked at their declaration sites by the typeParamHasAnnotation method for both class-level and method-level type parameters. The hasJSpecifyAnnotationDeep method is designed to check type uses (return types, parameters, etc.) and does not need a TYPEVAR case because type variable declaration bounds are already handled separately.
Applied to files:
jdk-javac-plugin/src/test/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializerTest.javajdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.java
📚 Learning: 2025-09-03T19:40:11.118Z
Learnt from: msridhar
Repo: uber/NullAway PR: 1276
File: nullaway/src/main/java/com/uber/nullaway/generics/TypeMetadataBuilder.java:235-241
Timestamp: 2025-09-03T19:40:11.118Z
Learning: In JDK 21+, ListBuffer can be passed directly to TypeMetadata.Annotations constructor without needing conversion to List via toList() method. The APIs were updated to make this compatible.
Applied to files:
jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.java
📚 Learning: 2025-12-31T23:59:20.009Z
Learnt from: msridhar
Repo: uber/NullAway PR: 1421
File: nullaway/src/main/java/com/uber/nullaway/NullAway.java:339-340
Timestamp: 2025-12-31T23:59:20.009Z
Learning: In Error Prone's ASTHelpers class, the getSymbol(MethodInvocationTree) overload is guaranteed to return a non-null Symbol.MethodSymbol. This differs from other overloads of getSymbol() that may return null. Therefore, no null check is required after calling ASTHelpers.getSymbol() with a MethodInvocationTree parameter.
Applied to files:
jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.java
🔇 Additional comments (8)
jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.java (3)
17-17: LGTM on imports and record update.The new imports for
NestedAnnotationInfoandSetare appropriate, and theMethodInforecord extension to includenestedAnnotationsListfollows a clean design pattern with-1for return type and0, 1, ...for parameter indices.Also applies to: 29-29, 54-60
176-186: LGTM on parameter nested annotation collection.The index-based iteration correctly maps parameter positions to their nested annotations, and the guard against null/empty sets prevents unnecessary map entries.
194-206: LGTM on MethodInfo construction and addition logic.The
nestedAnnotationsMapis correctly passed to theMethodInfoconstructor, and the existing filtering logic ensures only methods with JSpecify annotations are added.jdk-javac-plugin/src/test/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializerTest.java (5)
6-6: LGTM on new imports.All new imports are necessary for the ImmutableList deserializer and nested annotation test assertions.
Also applies to: 12-14, 18-20, 22-23, 29-30, 33-33
122-123: LGTM on existing test updates.The addition of
new HashMap<>()to existing test expectations correctly accommodates the newnestedAnnotationsListfield inMethodInfo.Also applies to: 157-158, 216-217, 253-254, 297-298, 304-305, 311-312
326-326: LGTM on annotationDeepScan test updates.The method name fix (
wildCard→wildcard) and the nested annotation assertions are well-structured. TheWILDCARD_BOUNDindex encoding (0 for extends, 1 for super) is applied consistently.Also applies to: 350-401
404-513: LGTM on new nestedAnnotationsForMethods test.Excellent test coverage for the nested annotation feature, including:
- Type argument annotations (
Class<@NonNull E>)- Return type nested annotations (
List<@Nullable String>)- Array element annotations (
String @Nullable [])- Combined array + type argument paths (
List<@NonNull String>[])- Wildcard extends bounds (
? extends @Nullable String)The test assertions correctly validate the type path construction for each scenario.
545-550: LGTM on Gson adapter registration.The
ImmutableListadapter is correctly registered with theGsonBuilderto enable proper deserialization of nested annotation type paths.
jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.java
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In
@jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/CreateNestedAnnotationInfoVisitor.java:
- Around line 23-29: Make the visitor's state immutable and avoid exposing
mutable internals: mark the fields path and nestedAnnotationInfoList as final
and rename nestedAnnotationInfoList → nestedAnnotationInfoSet for clarity; keep
path private/final (ArrayDeque<TypePathEntry>) and do not expose it. Wherever
the visitor currently returns its set (e.g., a getNestedAnnotationInfo or the
top-level visit method), return an immutable copy (e.g.,
Set.copyOf(nestedAnnotationInfoSet) or Collections.unmodifiableSet(new
HashSet<>(nestedAnnotationInfoSet))) instead of the internal collection so
callers cannot mutate internal state.
- Around line 122-148: The two methods hasNullableAnnotation and
hasNonNullAnnotation are duplicated; replace them with a single helper, e.g.,
hasAnnotation(TypeMirror type, String qualifiedName) (or
hasAnnotationByQualifiedName) that encapsulates the null-check and loop over
type.getAnnotationMirrors(), and then rewrite hasNullableAnnotation and
hasNonNullAnnotation to call that helper (or remove them and inline calls) using
constants for the two annotation names (e.g., JSPECIFY_NULLABLE =
"org.jspecify.annotations.Nullable" and JSPECIFY_NONNULL =
"org.jspecify.annotations.NonNull") to avoid repeated string literals; update
all usages in CreateNestedAnnotationInfoVisitor to use the new helper or
constants.
- Around line 75-114: In visitWildcardType in CreateNestedAnnotationInfoVisitor
the local variable nestedAnnotationInfo is reused across the upper and lower
bound blocks causing a stale value from the extends-bound to be added again for
an unannotated super-bound; fix by scoping/resetting nestedAnnotationInfo per
bound (e.g., declare a new nestedAnnotationInfo inside the if
(t.getExtendsBound() != null) block and again inside the if (t.getSuperBound()
!= null) block or explicitly set it to null before computing the lower-bound) so
only annotations found on the current bound produce a NestedAnnotationInfo added
to nestedAnnotationInfoList.
- Around line 31-54: Add a clarifying comment inside the visitClassType method
(in CreateNestedAnnotationInfoVisitor) stating that this method only processes
classType.getTypeArguments() and does not traverse or handle annotations on
enclosing/inner types (classType.getEnclosingType()), because the
TypePathEntry.Kind enum currently only includes ARRAY_ELEMENT, TYPE_ARGUMENT,
and WILDCARD_BOUND and does not represent inner-type positions; e.g., "Note:
does not yet handle annotations on enclosing types, e.g., Outer<@Nullable
T>.Inner".
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/CreateNestedAnnotationInfoVisitor.java
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: msridhar
Repo: uber/NullAway PR: 1248
File: nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java:847-857
Timestamp: 2025-08-28T04:54:20.953Z
Learning: In NullAway's GenericsChecks.java, NewClassTree support for explicit type argument substitution requires more extensive changes beyond just modifying the conditional in compareGenericTypeParameterNullabilityForCall. The maintainers prefer to handle NewClassTree support in a separate follow-up rather than expanding the scope of PRs focused on specific issues like super constructor calls.
Learnt from: haewiful
Repo: uber/NullAway PR: 1243
File: jdk-annotations/astubx-generator-cli/src/main/java/com/uber/nullaway/jdkannotations/AstubxGeneratorCLI.java:97-106
Timestamp: 2025-09-24T23:14:37.777Z
Learning: In the jdk-javac-plugin NullnessAnnotationSerializer, the plugin only creates one JSON file per compilation, so processing only the first JSON file is intentional behavior rather than a bug.
Learnt from: msridhar
Repo: uber/NullAway PR: 1316
File: jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.java:261-293
Timestamp: 2025-10-29T23:56:18.236Z
Learning: In NullAway's jdk-javac-plugin NullnessAnnotationSerializer, type variable bounds with annotations (e.g., `T extends Nullable Object`) are checked at their declaration sites by the typeParamHasAnnotation method for both class-level and method-level type parameters. The hasJSpecifyAnnotationDeep method is designed to check type uses (return types, parameters, etc.) and does not need a TYPEVAR case because type variable declaration bounds are already handled separately.
Learnt from: msridhar
Repo: uber/NullAway PR: 1245
File: guava-recent-unit-tests/src/test/java/com/uber/nullaway/guava/NullAwayGuavaParametricNullnessTests.java:101-102
Timestamp: 2025-08-14T18:50:06.159Z
Learning: In NullAway JSpecify tests, when JDK version requirements exist due to bytecode annotation reading capabilities, prefer failing tests over skipping them on unsupported versions to ensure CI catches regressions and enforces proper JDK version usage for developers.
Learnt from: msridhar
Repo: uber/NullAway PR: 1259
File: jdk-recent-unit-tests/src/test/java/com/uber/nullaway/jdk17/SwitchTests.java:318-321
Timestamp: 2025-08-29T18:41:43.584Z
Learning: Classes annotated with NullMarked are analyzed by NullAway even if they are not in packages specified by the AnnotatedPackages configuration. The NullMarked annotation guarantees NullAway analysis.
📚 Learning: 2025-08-28T04:54:20.953Z
Learnt from: msridhar
Repo: uber/NullAway PR: 1248
File: nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java:847-857
Timestamp: 2025-08-28T04:54:20.953Z
Learning: In NullAway's GenericsChecks.java, NewClassTree support for explicit type argument substitution requires more extensive changes beyond just modifying the conditional in compareGenericTypeParameterNullabilityForCall. The maintainers prefer to handle NewClassTree support in a separate follow-up rather than expanding the scope of PRs focused on specific issues like super constructor calls.
Applied to files:
jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/CreateNestedAnnotationInfoVisitor.java
📚 Learning: 2025-10-29T23:56:18.236Z
Learnt from: msridhar
Repo: uber/NullAway PR: 1316
File: jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.java:261-293
Timestamp: 2025-10-29T23:56:18.236Z
Learning: In NullAway's jdk-javac-plugin NullnessAnnotationSerializer, type variable bounds with annotations (e.g., `T extends Nullable Object`) are checked at their declaration sites by the typeParamHasAnnotation method for both class-level and method-level type parameters. The hasJSpecifyAnnotationDeep method is designed to check type uses (return types, parameters, etc.) and does not need a TYPEVAR case because type variable declaration bounds are already handled separately.
Applied to files:
jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/CreateNestedAnnotationInfoVisitor.java
📚 Learning: 2025-08-14T18:50:06.159Z
Learnt from: msridhar
Repo: uber/NullAway PR: 1245
File: guava-recent-unit-tests/src/test/java/com/uber/nullaway/guava/NullAwayGuavaParametricNullnessTests.java:101-102
Timestamp: 2025-08-14T18:50:06.159Z
Learning: In NullAway JSpecify tests, when JDK version requirements exist due to bytecode annotation reading capabilities, prefer failing tests over skipping them on unsupported versions to ensure CI catches regressions and enforces proper JDK version usage for developers.
Applied to files:
jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/CreateNestedAnnotationInfoVisitor.java
🔇 Additional comments (2)
jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/CreateNestedAnnotationInfoVisitor.java (2)
116-120: Code state does not match review comment—visitType already returns the accumulated set, not null.The actual implementation at line 118 already returns
nestedAnnotationInfoList(the accumulated set), notnull. All visitor methods (visitClassType, visitArrayType, visitWildcardType, visitType) follow the same pattern of accumulating results and returning the shared set. No changes are needed.The concern about wrapper types (AnnotatedType, ForAll, Union, Intersection) cannot be verified as these types do not appear to be used in the codebase.
Likely an incorrect or invalid review comment.
56-73: The code correctly handles both array reference and array element annotations. Tests confirm that javac places@Nullable String[]annotations on the element type (String), not the ArrayType node, so they are found during the recursive elemtype traversal, not the initial hasNullableAnnotation(t) check. TheString @Nullable []syntax (annotation on array itself) correctly records as nested with ARRAY_ELEMENT path. No fix required.Likely an incorrect or invalid review comment.
Comment on lines
23
to
29
| private ArrayDeque<TypePathEntry> path; | ||
| private Set<NestedAnnotationInfo> nestedAnnotationInfoList; | ||
|
|
||
| public CreateNestedAnnotationInfoVisitor() { | ||
| path = new ArrayDeque<>(); | ||
| nestedAnnotationInfoList = new HashSet<>(); | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Make visitor state safer (final fields + clearer naming + avoid exposing mutable internals).
path/nestedAnnotationInfoList are mutable instance state and the returned Set is the same object (callers can mutate it). At minimum: mark fields final, rename nestedAnnotationInfoList → nestedAnnotationInfoSet, and consider returning an immutable copy at the end of the top-level call.
Proposed diff
- private ArrayDeque<TypePathEntry> path;
- private Set<NestedAnnotationInfo> nestedAnnotationInfoList;
+ private final ArrayDeque<TypePathEntry> path = new ArrayDeque<>();
+ private final Set<NestedAnnotationInfo> nestedAnnotationInfoSet = new HashSet<>();
public CreateNestedAnnotationInfoVisitor() {
- path = new ArrayDeque<>();
- nestedAnnotationInfoList = new HashSet<>();
}🤖 Prompt for AI Agents
In
@jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/CreateNestedAnnotationInfoVisitor.java
around lines 23 - 29, Make the visitor's state immutable and avoid exposing
mutable internals: mark the fields path and nestedAnnotationInfoList as final
and rename nestedAnnotationInfoList → nestedAnnotationInfoSet for clarity; keep
path private/final (ArrayDeque<TypePathEntry>) and do not expose it. Wherever
the visitor currently returns its set (e.g., a getNestedAnnotationInfo or the
top-level visit method), return an immutable copy (e.g.,
Set.copyOf(nestedAnnotationInfoSet) or Collections.unmodifiableSet(new
HashSet<>(nestedAnnotationInfoSet))) instead of the internal collection so
callers cannot mutate internal state.
...ac-plugin/src/main/java/com/uber/nullaway/javacplugin/CreateNestedAnnotationInfoVisitor.java
Show resolved
Hide resolved
...ac-plugin/src/main/java/com/uber/nullaway/javacplugin/CreateNestedAnnotationInfoVisitor.java
Show resolved
Hide resolved
...ac-plugin/src/main/java/com/uber/nullaway/javacplugin/CreateNestedAnnotationInfoVisitor.java
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
@jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/CreateNestedAnnotationInfoVisitor.java:
- Around line 121-147: Remove the unreachable null checks in
hasNullableAnnotation(TypeMirror) and hasNonNullAnnotation(TypeMirror): delete
the "if (type == null) { return false; }" guards at the top of both methods
since TypeMirror is non-null under @NullMarked, leaving the existing iteration
over type.getAnnotationMirrors() and returns unchanged.
- Around line 76-114: The variable nestedAnnotationInfo is declared once at the
top of visitWildcardType and then reused for both the extends and super bound
logic; make nestedAnnotationInfo a locally-scoped variable inside each bound
block (the extends block handling t.getExtendsBound() and the super block
handling t.getSuperBound()) by removing the outer declaration and declaring a
new NestedAnnotationInfo nestedAnnotationInfo = null inside each block before
you check hasNullableAnnotation/hasNonNullAnnotation, leaving the rest of the
logic (adding to nestedAnnotationInfoList, calling accept, and removing path
entries) unchanged.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/CreateNestedAnnotationInfoVisitor.java
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: msridhar
Repo: uber/NullAway PR: 1248
File: nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java:847-857
Timestamp: 2025-08-28T04:54:20.953Z
Learning: In NullAway's GenericsChecks.java, NewClassTree support for explicit type argument substitution requires more extensive changes beyond just modifying the conditional in compareGenericTypeParameterNullabilityForCall. The maintainers prefer to handle NewClassTree support in a separate follow-up rather than expanding the scope of PRs focused on specific issues like super constructor calls.
Learnt from: haewiful
Repo: uber/NullAway PR: 1243
File: jdk-annotations/astubx-generator-cli/src/main/java/com/uber/nullaway/jdkannotations/AstubxGeneratorCLI.java:97-106
Timestamp: 2025-09-24T23:14:37.777Z
Learning: In the jdk-javac-plugin NullnessAnnotationSerializer, the plugin only creates one JSON file per compilation, so processing only the first JSON file is intentional behavior rather than a bug.
Learnt from: msridhar
Repo: uber/NullAway PR: 1245
File: guava-recent-unit-tests/src/test/java/com/uber/nullaway/guava/NullAwayGuavaParametricNullnessTests.java:101-102
Timestamp: 2025-08-14T18:50:06.159Z
Learning: In NullAway JSpecify tests, when JDK version requirements exist due to bytecode annotation reading capabilities, prefer failing tests over skipping them on unsupported versions to ensure CI catches regressions and enforces proper JDK version usage for developers.
Learnt from: msridhar
Repo: uber/NullAway PR: 1316
File: jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.java:261-293
Timestamp: 2025-10-29T23:56:18.236Z
Learning: In NullAway's jdk-javac-plugin NullnessAnnotationSerializer, type variable bounds with annotations (e.g., `T extends Nullable Object`) are checked at their declaration sites by the typeParamHasAnnotation method for both class-level and method-level type parameters. The hasJSpecifyAnnotationDeep method is designed to check type uses (return types, parameters, etc.) and does not need a TYPEVAR case because type variable declaration bounds are already handled separately.
Learnt from: msridhar
Repo: uber/NullAway PR: 1259
File: jdk-recent-unit-tests/src/test/java/com/uber/nullaway/jdk17/SwitchTests.java:318-321
Timestamp: 2025-08-29T18:41:43.584Z
Learning: Classes annotated with NullMarked are analyzed by NullAway even if they are not in packages specified by the AnnotatedPackages configuration. The NullMarked annotation guarantees NullAway analysis.
📚 Learning: 2025-08-28T04:54:20.953Z
Learnt from: msridhar
Repo: uber/NullAway PR: 1248
File: nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java:847-857
Timestamp: 2025-08-28T04:54:20.953Z
Learning: In NullAway's GenericsChecks.java, NewClassTree support for explicit type argument substitution requires more extensive changes beyond just modifying the conditional in compareGenericTypeParameterNullabilityForCall. The maintainers prefer to handle NewClassTree support in a separate follow-up rather than expanding the scope of PRs focused on specific issues like super constructor calls.
Applied to files:
jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/CreateNestedAnnotationInfoVisitor.java
📚 Learning: 2025-10-29T23:56:18.236Z
Learnt from: msridhar
Repo: uber/NullAway PR: 1316
File: jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.java:261-293
Timestamp: 2025-10-29T23:56:18.236Z
Learning: In NullAway's jdk-javac-plugin NullnessAnnotationSerializer, type variable bounds with annotations (e.g., `T extends Nullable Object`) are checked at their declaration sites by the typeParamHasAnnotation method for both class-level and method-level type parameters. The hasJSpecifyAnnotationDeep method is designed to check type uses (return types, parameters, etc.) and does not need a TYPEVAR case because type variable declaration bounds are already handled separately.
Applied to files:
jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/CreateNestedAnnotationInfoVisitor.java
📚 Learning: 2025-08-14T18:50:06.159Z
Learnt from: msridhar
Repo: uber/NullAway PR: 1245
File: guava-recent-unit-tests/src/test/java/com/uber/nullaway/guava/NullAwayGuavaParametricNullnessTests.java:101-102
Timestamp: 2025-08-14T18:50:06.159Z
Learning: In NullAway JSpecify tests, when JDK version requirements exist due to bytecode annotation reading capabilities, prefer failing tests over skipping them on unsupported versions to ensure CI catches regressions and enforces proper JDK version usage for developers.
Applied to files:
jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/CreateNestedAnnotationInfoVisitor.java
🔇 Additional comments (4)
jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/CreateNestedAnnotationInfoVisitor.java (4)
117-119: LGTM!The default
visitTypeimplementation correctly returns the accumulated results for types that don't need special traversal logic.
149-151: LGTM!The
getTypePathhelper correctly creates an immutable snapshot of the current path for inclusion inNestedAnnotationInfoinstances.
23-29: No action needed. The visitor is instantiated withnew CreateNestedAnnotationInfoVisitor()at each call site inNullnessAnnotationSerializer.java, ensuring fresh state for each type traversal. State accumulation is not a concern, and a reset method is unnecessary.Likely an incorrect or invalid review comment.
57-73: No issues identified. The logic for handling array type annotations is correct.The code properly distinguishes between:
String @Nullable []– annotation on the array dimension, captured with path[ARRAY_ELEMENT]@Nullable String[]– annotation on the element type, handled by recursive visitation ofelemtypeChecking annotations on the
ArrayTypeobject correctly identifies annotations on the array dimension itself (e.g.,String @Nullable []), and the subsequent recursive visitation oft.elemtypehandles annotations within the element type (e.g.,List<@Nullable String>). The path management is semantically sound.
...ac-plugin/src/main/java/com/uber/nullaway/javacplugin/CreateNestedAnnotationInfoVisitor.java
Outdated
Show resolved
Hide resolved
...ac-plugin/src/main/java/com/uber/nullaway/javacplugin/CreateNestedAnnotationInfoVisitor.java
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
@jdk-javac-plugin/src/test/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializerTest.java:
- Around line 518-537: The adapter immutableListAdapter currently throws on a
raw ImmutableList Type; change it to mirror AstubxGenerator by treating
non-parameterized ImmutableList as List<Object> instead of throwing: when
typeOfT is not a ParameterizedType, set the element/content type to Object (or a
generic Object Type) and proceed to deserialize each JsonElement with that
fallback type, then build the ImmutableList; keep the existing path for
ParameterizedType to use paramType.getActualTypeArguments()[0].
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
jdk-annotations/astubx-generator/src/main/java/com/uber/nullaway/jdkannotations/AstubxGenerator.javajdk-javac-plugin/build.gradlejdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/CreateNestedAnnotationInfoVisitor.javajdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NestedAnnotationInfo.javajdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.javajdk-javac-plugin/src/test/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializerTest.java
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: haewiful
Repo: uber/NullAway PR: 1243
File: jdk-annotations/astubx-generator-cli/src/main/java/com/uber/nullaway/jdkannotations/AstubxGeneratorCLI.java:97-106
Timestamp: 2025-09-24T23:14:37.777Z
Learning: In the jdk-javac-plugin NullnessAnnotationSerializer, the plugin only creates one JSON file per compilation, so processing only the first JSON file is intentional behavior rather than a bug.
Learnt from: msridhar
Repo: uber/NullAway PR: 1248
File: nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java:847-857
Timestamp: 2025-08-28T04:54:20.953Z
Learning: In NullAway's GenericsChecks.java, NewClassTree support for explicit type argument substitution requires more extensive changes beyond just modifying the conditional in compareGenericTypeParameterNullabilityForCall. The maintainers prefer to handle NewClassTree support in a separate follow-up rather than expanding the scope of PRs focused on specific issues like super constructor calls.
Learnt from: msridhar
Repo: uber/NullAway PR: 1316
File: jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.java:261-293
Timestamp: 2025-10-29T23:56:18.236Z
Learning: In NullAway's jdk-javac-plugin NullnessAnnotationSerializer, type variable bounds with annotations (e.g., `T extends Nullable Object`) are checked at their declaration sites by the typeParamHasAnnotation method for both class-level and method-level type parameters. The hasJSpecifyAnnotationDeep method is designed to check type uses (return types, parameters, etc.) and does not need a TYPEVAR case because type variable declaration bounds are already handled separately.
Learnt from: msridhar
Repo: uber/NullAway PR: 1245
File: guava-recent-unit-tests/src/test/java/com/uber/nullaway/guava/NullAwayGuavaParametricNullnessTests.java:101-102
Timestamp: 2025-08-14T18:50:06.159Z
Learning: In NullAway JSpecify tests, when JDK version requirements exist due to bytecode annotation reading capabilities, prefer failing tests over skipping them on unsupported versions to ensure CI catches regressions and enforces proper JDK version usage for developers.
📚 Learning: 2025-08-14T18:50:06.159Z
Learnt from: msridhar
Repo: uber/NullAway PR: 1245
File: guava-recent-unit-tests/src/test/java/com/uber/nullaway/guava/NullAwayGuavaParametricNullnessTests.java:101-102
Timestamp: 2025-08-14T18:50:06.159Z
Learning: In NullAway JSpecify tests, when JDK version requirements exist due to bytecode annotation reading capabilities, prefer failing tests over skipping them on unsupported versions to ensure CI catches regressions and enforces proper JDK version usage for developers.
Applied to files:
jdk-javac-plugin/src/test/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializerTest.javajdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NestedAnnotationInfo.javajdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/CreateNestedAnnotationInfoVisitor.javajdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.java
📚 Learning: 2025-09-24T23:14:37.777Z
Learnt from: haewiful
Repo: uber/NullAway PR: 1243
File: jdk-annotations/astubx-generator-cli/src/main/java/com/uber/nullaway/jdkannotations/AstubxGeneratorCLI.java:97-106
Timestamp: 2025-09-24T23:14:37.777Z
Learning: In the jdk-javac-plugin NullnessAnnotationSerializer, the plugin only creates one JSON file per compilation, so processing only the first JSON file is intentional behavior rather than a bug.
Applied to files:
jdk-javac-plugin/src/test/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializerTest.javajdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NestedAnnotationInfo.javajdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.javajdk-annotations/astubx-generator/src/main/java/com/uber/nullaway/jdkannotations/AstubxGenerator.java
📚 Learning: 2025-08-28T04:54:20.953Z
Learnt from: msridhar
Repo: uber/NullAway PR: 1248
File: nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java:847-857
Timestamp: 2025-08-28T04:54:20.953Z
Learning: In NullAway's GenericsChecks.java, NewClassTree support for explicit type argument substitution requires more extensive changes beyond just modifying the conditional in compareGenericTypeParameterNullabilityForCall. The maintainers prefer to handle NewClassTree support in a separate follow-up rather than expanding the scope of PRs focused on specific issues like super constructor calls.
Applied to files:
jdk-javac-plugin/src/test/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializerTest.javajdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NestedAnnotationInfo.javajdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/CreateNestedAnnotationInfoVisitor.javajdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.javajdk-annotations/astubx-generator/src/main/java/com/uber/nullaway/jdkannotations/AstubxGenerator.java
📚 Learning: 2025-10-29T23:56:18.236Z
Learnt from: msridhar
Repo: uber/NullAway PR: 1316
File: jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.java:261-293
Timestamp: 2025-10-29T23:56:18.236Z
Learning: In NullAway's jdk-javac-plugin NullnessAnnotationSerializer, type variable bounds with annotations (e.g., `T extends Nullable Object`) are checked at their declaration sites by the typeParamHasAnnotation method for both class-level and method-level type parameters. The hasJSpecifyAnnotationDeep method is designed to check type uses (return types, parameters, etc.) and does not need a TYPEVAR case because type variable declaration bounds are already handled separately.
Applied to files:
jdk-javac-plugin/src/test/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializerTest.javajdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NestedAnnotationInfo.javajdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/CreateNestedAnnotationInfoVisitor.javajdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.java
📚 Learning: 2025-08-29T18:41:43.584Z
Learnt from: msridhar
Repo: uber/NullAway PR: 1259
File: jdk-recent-unit-tests/src/test/java/com/uber/nullaway/jdk17/SwitchTests.java:318-321
Timestamp: 2025-08-29T18:41:43.584Z
Learning: Classes annotated with NullMarked are analyzed by NullAway even if they are not in packages specified by the AnnotatedPackages configuration. The NullMarked annotation guarantees NullAway analysis.
Applied to files:
jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NestedAnnotationInfo.javajdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/CreateNestedAnnotationInfoVisitor.java
📚 Learning: 2025-09-03T19:40:11.118Z
Learnt from: msridhar
Repo: uber/NullAway PR: 1276
File: nullaway/src/main/java/com/uber/nullaway/generics/TypeMetadataBuilder.java:235-241
Timestamp: 2025-09-03T19:40:11.118Z
Learning: In JDK 21+, ListBuffer can be passed directly to TypeMetadata.Annotations constructor without needing conversion to List via toList() method. The APIs were updated to make this compatible.
Applied to files:
jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.java
📚 Learning: 2025-12-31T23:59:20.009Z
Learnt from: msridhar
Repo: uber/NullAway PR: 1421
File: nullaway/src/main/java/com/uber/nullaway/NullAway.java:339-340
Timestamp: 2025-12-31T23:59:20.009Z
Learning: In Error Prone's ASTHelpers class, the getSymbol(MethodInvocationTree) overload is guaranteed to return a non-null Symbol.MethodSymbol. This differs from other overloads of getSymbol() that may return null. Therefore, no null check is required after calling ASTHelpers.getSymbol() with a MethodInvocationTree parameter.
Applied to files:
jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.java
🔇 Additional comments (10)
jdk-javac-plugin/build.gradle (1)
60-60: LGTM!The Guava dependency is appropriately added to support
ImmutableListusage in the newNestedAnnotationInfoandCreateNestedAnnotationInfoVisitorclasses.jdk-annotations/astubx-generator/src/main/java/com/uber/nullaway/jdkannotations/AstubxGenerator.java (1)
171-199: LGTM!The
ImmutableListtype adapter is well-implemented, correctly handling null JSON values, raw types, and parameterized types. This is necessary since Gson doesn't natively support Guava's immutable collections.jdk-javac-plugin/src/test/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializerTest.java (1)
403-511: Good test coverage for nested annotations.The new
nestedAnnotationsForMethods()test comprehensively covers multiple scenarios:
- Type argument annotations with
@NonNull- Return type nested annotations (index
-1)- Parameter array annotations with nested
List<@NonNull String>[]- Wildcard upper bound annotations
The wildcard indices correctly use
0forextendsand1forsuperper theTypePathEntrydocumentation.jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NestedAnnotationInfo.java (1)
1-42: Well-designed data model.The record-based design is clean and appropriately immutable. The Javadoc clearly documents the semantics of the
indexfield for eachKind, which is essential for consumers of this API.jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.java (2)
158-186: LGTM!The nested annotation collection logic is well-structured:
- Return type annotations stored at key
-1- Parameter annotations stored at their respective indices
- A new
CreateNestedAnnotationInfoVisitorinstance is correctly created for each type to avoid state mixing
53-59: LGTM!The
MethodInforecord is appropriately extended with thenestedAnnotationsListfield to carry nested annotation metadata for both return types and parameters.jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/CreateNestedAnnotationInfoVisitor.java (4)
33-57: LGTM!The
visitClassTypemethod correctly traverses type arguments, maintaining the path stack and recording annotations with their corresponding type paths. The recursive call on line 52 ensures nested generic types are also processed.
78-118: LGTM!The wildcard handling correctly distinguishes between upper bounds (
extends, index 0) and lower bounds (super, index 1), matching the documented semantics inTypePathEntry. Both bounds are processed with appropriate path tracking.
125-137: Defensive null check is appropriate.The null check on line 126 is defensive since
TypeMirrorparameters from bound accessors (likegetExtendsBound()) can potentially be null. ThehasAnnotationmethod is static and correctly handles this edge case.
59-76: The implementation is correct as-is. The design intentionally separates annotation tracking based on position:
- Annotations on the element type like
@Nullable String[]are captured in the serialized type string ([email protected] String[]) viatoString()and do not require path-basedNestedAnnotationInfoentries.- Annotations on the array type itself like
String @Nullable []are captured with path-based tracking (NestedAnnotationInfowithARRAY_ELEMENTpath entry).This is consistent with JVMS attribute encoding: nested annotations on base types don't need positional tracking since they're part of the canonical type representation. The visitor correctly recurses into
elemtypebut only records path-based info for structural positions (array, wildcard, type argument).
Comment on lines
+518
to
+537
| JsonDeserializer<ImmutableList<?>> immutableListAdapter = | ||
| (json, typeOfT, context) -> { | ||
| ImmutableList.Builder<Object> builder = ImmutableList.builder(); | ||
|
|
||
| if (json.isJsonArray()) { | ||
| JsonArray array = json.getAsJsonArray(); | ||
| // Get the component type of list (with defensive check) | ||
| if (!(typeOfT instanceof ParameterizedType paramType)) { | ||
| throw new IllegalArgumentException("Expected parameterized ImmutableList type"); | ||
| } | ||
| Type contentClass = paramType.getActualTypeArguments()[0]; | ||
|
|
||
| for (JsonElement element : array) { | ||
| // Deserialize the inner element using Gson's context | ||
| Object value = context.deserialize(element, contentClass); | ||
| builder.add(value); | ||
| } | ||
| } | ||
| return builder.build(); | ||
| }; |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider aligning the adapter behavior with AstubxGenerator.
This adapter throws an exception for non-parameterized ImmutableList types, while the adapter in AstubxGenerator gracefully handles this case by deserializing as List<Object>. While tests may never encounter raw ImmutableList types, aligning the behavior would improve consistency across the codebase.
♻️ Suggested alignment
(json, typeOfT, context) -> {
ImmutableList.Builder<Object> builder = ImmutableList.builder();
if (json.isJsonArray()) {
JsonArray array = json.getAsJsonArray();
// Get the component type of list (with defensive check)
if (!(typeOfT instanceof ParameterizedType paramType)) {
- throw new IllegalArgumentException("Expected parameterized ImmutableList type");
+ // Raw ImmutableList, deserialize as List<Object>
+ for (JsonElement element : array) {
+ Object value = context.deserialize(element, Object.class);
+ builder.add(value);
+ }
+ return builder.build();
}
Type contentClass = paramType.getActualTypeArguments()[0];
for (JsonElement element : array) {
// Deserialize the inner element using Gson's context
Object value = context.deserialize(element, contentClass);
builder.add(value);
}
}
return builder.build();
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| JsonDeserializer<ImmutableList<?>> immutableListAdapter = | |
| (json, typeOfT, context) -> { | |
| ImmutableList.Builder<Object> builder = ImmutableList.builder(); | |
| if (json.isJsonArray()) { | |
| JsonArray array = json.getAsJsonArray(); | |
| // Get the component type of list (with defensive check) | |
| if (!(typeOfT instanceof ParameterizedType paramType)) { | |
| throw new IllegalArgumentException("Expected parameterized ImmutableList type"); | |
| } | |
| Type contentClass = paramType.getActualTypeArguments()[0]; | |
| for (JsonElement element : array) { | |
| // Deserialize the inner element using Gson's context | |
| Object value = context.deserialize(element, contentClass); | |
| builder.add(value); | |
| } | |
| } | |
| return builder.build(); | |
| }; | |
| JsonDeserializer<ImmutableList<?>> immutableListAdapter = | |
| (json, typeOfT, context) -> { | |
| ImmutableList.Builder<Object> builder = ImmutableList.builder(); | |
| if (json.isJsonArray()) { | |
| JsonArray array = json.getAsJsonArray(); | |
| // Get the component type of list (with defensive check) | |
| if (!(typeOfT instanceof ParameterizedType paramType)) { | |
| // Raw ImmutableList, deserialize as List<Object> | |
| for (JsonElement element : array) { | |
| Object value = context.deserialize(element, Object.class); | |
| builder.add(value); | |
| } | |
| return builder.build(); | |
| } | |
| Type contentClass = paramType.getActualTypeArguments()[0]; | |
| for (JsonElement element : array) { | |
| // Deserialize the inner element using Gson's context | |
| Object value = context.deserialize(element, contentClass); | |
| builder.add(value); | |
| } | |
| } | |
| return builder.build(); | |
| }; |
🤖 Prompt for AI Agents
In
@jdk-javac-plugin/src/test/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializerTest.java
around lines 518 - 537, The adapter immutableListAdapter currently throws on a
raw ImmutableList Type; change it to mirror AstubxGenerator by treating
non-parameterized ImmutableList as List<Object> instead of throwing: when
typeOfT is not a ParameterizedType, set the element/content type to Object (or a
generic Object Type) and proceed to deserialize each JsonElement with that
fallback type, then build the ImmutableList; keep the existing path for
ParameterizedType to use paramType.getActualTypeArguments()[0].
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1432 +/- ##
=========================================
Coverage 88.36% 88.37%
- Complexity 2681 2702 +21
=========================================
Files 97 99 +2
Lines 8882 8965 +83
Branches 1779 1790 +11
=========================================
+ Hits 7849 7923 +74
- Misses 511 516 +5
- Partials 522 526 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
msridhar
reviewed
Jan 14, 2026
Collaborator
msridhar
left a comment
msridhar
left a comment
There was a problem hiding this comment.
Looks promising! Some comments:
jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NestedAnnotationInfo.java
Show resolved
Hide resolved
...ac-plugin/src/main/java/com/uber/nullaway/javacplugin/CreateNestedAnnotationInfoVisitor.java
Show resolved
Hide resolved
| for (int idx = 0; idx < typeArguments.size(); idx++) { | ||
| path.addLast(new TypePathEntry(TypePathEntry.Kind.TYPE_ARGUMENT, idx)); | ||
| Type typeArg = typeArguments.get(idx); | ||
| ImmutableList<TypePathEntry> typePath = getTypePath(); |
Collaborator
There was a problem hiding this comment.
Let's not store this in a local variable here, since it will do a copy of path for every type variable, whether it is annotated or not. Instead, just call getTypePath() at lines 45 and 47
...ac-plugin/src/main/java/com/uber/nullaway/javacplugin/CreateNestedAnnotationInfoVisitor.java
Outdated
Show resolved
Hide resolved
|
|
||
| @NullMarked | ||
| public class CreateNestedAnnotationInfoVisitor | ||
| extends Types.DefaultTypeVisitor<Set<NestedAnnotationInfo>, @Nullable Void> { |
Collaborator
There was a problem hiding this comment.
Since we're storing the nestedAnnotationInfoSet in a field, I don't think we need to return it from every visitor method. Instead, make this:
Suggested change
| extends Types.DefaultTypeVisitor<Set<NestedAnnotationInfo>, @Nullable Void> { | |
| extends Types.DefaultTypeVisitor<@Nullable Void, @Nullable Void> { |
Then, return null from every visitXXX method, but add a getter method getNestedAnnotationInfoSet() so that the client can retrieve the set at the end.
| new HashMap<>( | ||
| Map.of( | ||
| 0, | ||
| new HashSet<>( |
Collaborator
There was a problem hiding this comment.
You shouldn't need this? The Set.of result should be enough?
| new HashMap<>( | ||
| Map.of( | ||
| 0, | ||
| new HashSet<>( |
Collaborator
There was a problem hiding this comment.
Again I don't think this is needed
| false, | ||
| false, | ||
| List.of(new TypeParamInfo("E", List.of("@Nullable Object"))), | ||
| new HashMap<>( |
Collaborator
There was a problem hiding this comment.
In general, we shouldn't need to ever do new HashMap<>(Map.of(...)) or new HashSet<>(Set.of(...)) in this test code I think...
Comment on lines
474
to
481
| new HashSet<>( | ||
| Set.of( | ||
| new NestedAnnotationInfo( | ||
| Annotation.NULLABLE, | ||
| ImmutableList.of( | ||
| new TypePathEntry( | ||
| TypePathEntry.Kind.ARRAY_ELEMENT, | ||
| -1))))), |
Collaborator
There was a problem hiding this comment.
I think this is wrong! If the array element type was @Nullable, that would be written as @Nullable String [], not String @Nullable []. See here. Please fix.
...tations/astubx-generator/src/main/java/com/uber/nullaway/jdkannotations/AstubxGenerator.java
Show resolved
Hide resolved
Co-authored-by: Manu Sridharan <[email protected]>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/CreateNestedAnnotationInfoVisitor.java`:
- Around line 42-47: Remove the stray closing parenthesis and eliminate the
unused local by using the computed typePath variable: replace calls to
getTypePath() inside the nestedAnnotationInfoSet.add(...) invocations with the
previously declared ImmutableList<TypePathEntry> typePath, and remove the extra
')' after new NestedAnnotationInfo(Annotation.NONNULL, getTypePath()) so both
branches add new NestedAnnotationInfo(Annotation.NULLABLE, typePath) or new
NestedAnnotationInfo(Annotation.NONNULL, typePath); keep existing checks
hasNullableAnnotation(typeArg), hasNonNullAnnotation(typeArg), and the
nestedAnnotationInfoSet as-is.
♻️ Duplicate comments (3)
jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/CreateNestedAnnotationInfoVisitor.java (3)
18-20: Add class-level Javadoc.The class is missing documentation explaining its purpose and usage. Per past review feedback, add Javadoc describing that this visitor traverses Java type structures to collect nested JSpecify nullability annotations into a
Set<NestedAnnotationInfo>.
55-72: Simplify to match the pattern suggested forvisitClassType.Per past review feedback, this method can be simplified by removing the intermediate
nestedAnnotationInfovariable and directly adding to the set when an annotation is found.♻️ Proposed simplification
public `@Nullable` Set<NestedAnnotationInfo> visitArrayType( Type.ArrayType t, `@Nullable` Void unused) { path.addLast(new TypePathEntry(TypePathEntry.Kind.ARRAY_ELEMENT, -1)); - ImmutableList<TypePathEntry> typePath = getTypePath(); - NestedAnnotationInfo nestedAnnotationInfo = null; if (hasNullableAnnotation(t)) { - nestedAnnotationInfo = new NestedAnnotationInfo(Annotation.NULLABLE, typePath); + nestedAnnotationInfoSet.add(new NestedAnnotationInfo(Annotation.NULLABLE, getTypePath())); } else if (hasNonNullAnnotation(t)) { - nestedAnnotationInfo = new NestedAnnotationInfo(Annotation.NONNULL, typePath); - } - if (nestedAnnotationInfo != null) { - nestedAnnotationInfoSet.add(nestedAnnotationInfo); + nestedAnnotationInfoSet.add(new NestedAnnotationInfo(Annotation.NONNULL, getTypePath())); } t.elemtype.accept(this, null); path.removeLast(); return nestedAnnotationInfoSet; }
74-114: Extract repeated annotation-checking logic to a helper method.The pattern of checking for Nullable/NonNull annotations and adding to
nestedAnnotationInfoSetis repeated multiple times (invisitClassType,visitArrayType, and twice here invisitWildcardType). Per past review feedback, extract this to a helper method.♻️ Proposed helper method and refactored visitWildcardType
Add a helper method:
private void addNestedAnnotationIfPresent(TypeMirror type) { if (hasNullableAnnotation(type)) { nestedAnnotationInfoSet.add(new NestedAnnotationInfo(Annotation.NULLABLE, getTypePath())); } else if (hasNonNullAnnotation(type)) { nestedAnnotationInfoSet.add(new NestedAnnotationInfo(Annotation.NONNULL, getTypePath())); } }Then simplify
visitWildcardType:public `@Nullable` Set<NestedAnnotationInfo> visitWildcardType( Type.WildcardType t, `@Nullable` Void unused) { // Upper Bound (? extends T) if (t.getExtendsBound() != null) { - NestedAnnotationInfo nestedAnnotationInfo = null; path.addLast(new TypePathEntry(TypePathEntry.Kind.WILDCARD_BOUND, 0)); - ImmutableList<TypePathEntry> typePath = getTypePath(); - Type upperBound = t.getExtendsBound(); - if (hasNullableAnnotation(upperBound)) { - nestedAnnotationInfo = new NestedAnnotationInfo(Annotation.NULLABLE, typePath); - } else if (hasNonNullAnnotation(upperBound)) { - nestedAnnotationInfo = new NestedAnnotationInfo(Annotation.NONNULL, typePath); - } - if (nestedAnnotationInfo != null) { - nestedAnnotationInfoSet.add(nestedAnnotationInfo); - } + addNestedAnnotationIfPresent(t.getExtendsBound()); t.getExtendsBound().accept(this, null); path.removeLast(); } // Lower Bound (? super T) if (t.getSuperBound() != null) { - NestedAnnotationInfo nestedAnnotationInfo = null; path.addLast(new TypePathEntry(TypePathEntry.Kind.WILDCARD_BOUND, 1)); - ImmutableList<TypePathEntry> typePath = getTypePath(); - Type lowerBound = t.getSuperBound(); - if (hasNullableAnnotation(lowerBound)) { - nestedAnnotationInfo = new NestedAnnotationInfo(Annotation.NULLABLE, typePath); - } else if (hasNonNullAnnotation(lowerBound)) { - nestedAnnotationInfo = new NestedAnnotationInfo(Annotation.NONNULL, typePath); - } - if (nestedAnnotationInfo != null) { - nestedAnnotationInfoSet.add(nestedAnnotationInfo); - } + addNestedAnnotationIfPresent(t.getSuperBound()); t.getSuperBound().accept(this, null); path.removeLast(); } return nestedAnnotationInfoSet; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/CreateNestedAnnotationInfoVisitor.java
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: msridhar
Repo: uber/NullAway PR: 1248
File: nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java:847-857
Timestamp: 2025-08-28T04:54:20.953Z
Learning: In NullAway's GenericsChecks.java, NewClassTree support for explicit type argument substitution requires more extensive changes beyond just modifying the conditional in compareGenericTypeParameterNullabilityForCall. The maintainers prefer to handle NewClassTree support in a separate follow-up rather than expanding the scope of PRs focused on specific issues like super constructor calls.
Learnt from: msridhar
Repo: uber/NullAway PR: 1316
File: jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.java:261-293
Timestamp: 2025-10-29T23:56:18.236Z
Learning: In NullAway's jdk-javac-plugin NullnessAnnotationSerializer, type variable bounds with annotations (e.g., `T extends Nullable Object`) are checked at their declaration sites by the typeParamHasAnnotation method for both class-level and method-level type parameters. The hasJSpecifyAnnotationDeep method is designed to check type uses (return types, parameters, etc.) and does not need a TYPEVAR case because type variable declaration bounds are already handled separately.
Learnt from: msridhar
Repo: uber/NullAway PR: 1245
File: guava-recent-unit-tests/src/test/java/com/uber/nullaway/guava/NullAwayGuavaParametricNullnessTests.java:101-102
Timestamp: 2025-08-14T18:50:06.159Z
Learning: In NullAway JSpecify tests, when JDK version requirements exist due to bytecode annotation reading capabilities, prefer failing tests over skipping them on unsupported versions to ensure CI catches regressions and enforces proper JDK version usage for developers.
Learnt from: haewiful
Repo: uber/NullAway PR: 1243
File: jdk-annotations/astubx-generator-cli/src/main/java/com/uber/nullaway/jdkannotations/AstubxGeneratorCLI.java:97-106
Timestamp: 2025-09-24T23:14:37.777Z
Learning: In the jdk-javac-plugin NullnessAnnotationSerializer, the plugin only creates one JSON file per compilation, so processing only the first JSON file is intentional behavior rather than a bug.
📚 Learning: 2025-08-28T04:54:20.953Z
Learnt from: msridhar
Repo: uber/NullAway PR: 1248
File: nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java:847-857
Timestamp: 2025-08-28T04:54:20.953Z
Learning: In NullAway's GenericsChecks.java, NewClassTree support for explicit type argument substitution requires more extensive changes beyond just modifying the conditional in compareGenericTypeParameterNullabilityForCall. The maintainers prefer to handle NewClassTree support in a separate follow-up rather than expanding the scope of PRs focused on specific issues like super constructor calls.
Applied to files:
jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/CreateNestedAnnotationInfoVisitor.java
📚 Learning: 2025-10-29T23:56:18.236Z
Learnt from: msridhar
Repo: uber/NullAway PR: 1316
File: jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.java:261-293
Timestamp: 2025-10-29T23:56:18.236Z
Learning: In NullAway's jdk-javac-plugin NullnessAnnotationSerializer, type variable bounds with annotations (e.g., `T extends Nullable Object`) are checked at their declaration sites by the typeParamHasAnnotation method for both class-level and method-level type parameters. The hasJSpecifyAnnotationDeep method is designed to check type uses (return types, parameters, etc.) and does not need a TYPEVAR case because type variable declaration bounds are already handled separately.
Applied to files:
jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/CreateNestedAnnotationInfoVisitor.java
📚 Learning: 2025-08-14T18:50:06.159Z
Learnt from: msridhar
Repo: uber/NullAway PR: 1245
File: guava-recent-unit-tests/src/test/java/com/uber/nullaway/guava/NullAwayGuavaParametricNullnessTests.java:101-102
Timestamp: 2025-08-14T18:50:06.159Z
Learning: In NullAway JSpecify tests, when JDK version requirements exist due to bytecode annotation reading capabilities, prefer failing tests over skipping them on unsupported versions to ensure CI catches regressions and enforces proper JDK version usage for developers.
Applied to files:
jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/CreateNestedAnnotationInfoVisitor.java
📚 Learning: 2025-08-29T18:41:43.584Z
Learnt from: msridhar
Repo: uber/NullAway PR: 1259
File: jdk-recent-unit-tests/src/test/java/com/uber/nullaway/jdk17/SwitchTests.java:318-321
Timestamp: 2025-08-29T18:41:43.584Z
Learning: Classes annotated with NullMarked are analyzed by NullAway even if they are not in packages specified by the AnnotatedPackages configuration. The NullMarked annotation guarantees NullAway analysis.
Applied to files:
jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/CreateNestedAnnotationInfoVisitor.java
🔇 Additional comments (3)
jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/CreateNestedAnnotationInfoVisitor.java (3)
1-16: LGTM!Imports are appropriate and all appear to be used within the class.
116-119: LGTM!The fallback
visitTypecorrectly returns the accumulated set for types that don't need special handling.
121-145: LGTM!The helper methods are well-structured. The consolidated
hasAnnotationmethod with constants avoids code duplication, andgetTypePath()correctly returns an immutable copy of the path.The null check at line 122 is technically unreachable under
@NullMarked, but sincehasAnnotationis static it provides defensive coding if called from other contexts.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Comment on lines
42
to
47
| ImmutableList<TypePathEntry> typePath = getTypePath(); | ||
| if (hasNullableAnnotation(typeArg)) { | ||
| nestedAnnotationInfoSet.add(new NestedAnnotationInfo(Annotation.NULLABLE, getTypePath())); | ||
| } else if (hasNonNullAnnotation(typeArg)) { | ||
| nestedAnnotationInfoSet.add(new NestedAnnotationInfo(Annotation.NONNULL, getTypePath()))); | ||
| } |
Contributor
There was a problem hiding this comment.
Syntax error and unused variable.
Two issues in this block:
- Critical: Line 46 has an extra closing parenthesis that will cause a compilation error.
- Line 42 declares
typePathwhich is never used (lines 44-46 callgetTypePath()directly).
🐛 Proposed fix
- ImmutableList<TypePathEntry> typePath = getTypePath();
if (hasNullableAnnotation(typeArg)) {
nestedAnnotationInfoSet.add(new NestedAnnotationInfo(Annotation.NULLABLE, getTypePath()));
} else if (hasNonNullAnnotation(typeArg)) {
- nestedAnnotationInfoSet.add(new NestedAnnotationInfo(Annotation.NONNULL, getTypePath())));
+ nestedAnnotationInfoSet.add(new NestedAnnotationInfo(Annotation.NONNULL, getTypePath()));
}🤖 Prompt for AI Agents
In
`@jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/CreateNestedAnnotationInfoVisitor.java`
around lines 42 - 47, Remove the stray closing parenthesis and eliminate the
unused local by using the computed typePath variable: replace calls to
getTypePath() inside the nestedAnnotationInfoSet.add(...) invocations with the
previously declared ImmutableList<TypePathEntry> typePath, and remove the extra
')' after new NestedAnnotationInfo(Annotation.NONNULL, getTypePath()) so both
branches add new NestedAnnotationInfo(Annotation.NULLABLE, typePath) or new
NestedAnnotationInfo(Annotation.NONNULL, typePath); keep existing checks
hasNullableAnnotation(typeArg), hasNonNullAnnotation(typeArg), and the
nestedAnnotationInfoSet as-is.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/CreateNestedAnnotationInfoVisitor.java`:
- Around line 86-88: getNestedAnnotationInfoSet currently exposes the internal
mutable HashSet allowing external mutation; change the getter in
CreateNestedAnnotationInfoVisitor to return an unmodifiable view (e.g.,
Collections.unmodifiableSet(nestedAnnotationInfoSet) or
Set.copyOf(nestedAnnotationInfoSet)) so callers cannot modify the visitor's
internal state, keeping the field type and name (nestedAnnotationInfoSet,
getNestedAnnotationInfoSet) unchanged.
♻️ Duplicate comments (4)
jdk-javac-plugin/src/test/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializerTest.java (1)
496-515: Consider returning an empty list for non-parameterized types instead of throwing.The adapter throws when
typeOfTis not aParameterizedType. While this may never occur in tests, gracefully handling it (similar toAstubxGenerator) would improve robustness.jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/CreateNestedAnnotationInfoVisitor.java (3)
33-47: Add a comment clarifying that enclosing types are not traversed.As noted in a previous review,
visitClassTypeonly processesgetTypeArguments()and does not handle annotations on enclosing types (e.g.,Outer<@Nullable T>.Inner). A brief comment would clarify this intentional limitation.
98-110: Remove redundant null check.The
typeparameter has typeTypeMirror(non-null under@NullMarked), making the null check at lines 99-101 unreachable.🧹 Proposed fix
private static boolean hasAnnotation(TypeMirror type, String qname) { - if (type == null) { - return false; - } for (AnnotationMirror annotation : type.getAnnotationMirrors()) {
18-20: Add class-level Javadoc.The class is missing documentation explaining its purpose. A brief Javadoc describing that this visitor collects nested nullability annotations from Java types would be helpful.
📝 Suggested Javadoc
`@NullMarked` +/** + * A type visitor that collects nested JSpecify nullability annotations ({`@code` `@Nullable`} and + * {`@code` `@NonNull`}) from complex Java types such as parameterized types, arrays, and wildcards. + * The collected annotations are paired with their type paths for later serialization. + */ public class CreateNestedAnnotationInfoVisitor extends Types.DefaultTypeVisitor<@Nullable Void, `@Nullable` Void> {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/CreateNestedAnnotationInfoVisitor.javajdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.javajdk-javac-plugin/src/test/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializerTest.java
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: haewiful
Repo: uber/NullAway PR: 1243
File: jdk-annotations/astubx-generator-cli/src/main/java/com/uber/nullaway/jdkannotations/AstubxGeneratorCLI.java:97-106
Timestamp: 2025-09-24T23:14:37.777Z
Learning: In the jdk-javac-plugin NullnessAnnotationSerializer, the plugin only creates one JSON file per compilation, so processing only the first JSON file is intentional behavior rather than a bug.
Learnt from: msridhar
Repo: uber/NullAway PR: 1248
File: nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java:847-857
Timestamp: 2025-08-28T04:54:20.953Z
Learning: In NullAway's GenericsChecks.java, NewClassTree support for explicit type argument substitution requires more extensive changes beyond just modifying the conditional in compareGenericTypeParameterNullabilityForCall. The maintainers prefer to handle NewClassTree support in a separate follow-up rather than expanding the scope of PRs focused on specific issues like super constructor calls.
Learnt from: msridhar
Repo: uber/NullAway PR: 1316
File: jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.java:261-293
Timestamp: 2025-10-29T23:56:18.236Z
Learning: In NullAway's jdk-javac-plugin NullnessAnnotationSerializer, type variable bounds with annotations (e.g., `T extends Nullable Object`) are checked at their declaration sites by the typeParamHasAnnotation method for both class-level and method-level type parameters. The hasJSpecifyAnnotationDeep method is designed to check type uses (return types, parameters, etc.) and does not need a TYPEVAR case because type variable declaration bounds are already handled separately.
Learnt from: msridhar
Repo: uber/NullAway PR: 1245
File: guava-recent-unit-tests/src/test/java/com/uber/nullaway/guava/NullAwayGuavaParametricNullnessTests.java:101-102
Timestamp: 2025-08-14T18:50:06.159Z
Learning: In NullAway JSpecify tests, when JDK version requirements exist due to bytecode annotation reading capabilities, prefer failing tests over skipping them on unsupported versions to ensure CI catches regressions and enforces proper JDK version usage for developers.
📚 Learning: 2025-10-29T23:56:18.236Z
Learnt from: msridhar
Repo: uber/NullAway PR: 1316
File: jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.java:261-293
Timestamp: 2025-10-29T23:56:18.236Z
Learning: In NullAway's jdk-javac-plugin NullnessAnnotationSerializer, type variable bounds with annotations (e.g., `T extends Nullable Object`) are checked at their declaration sites by the typeParamHasAnnotation method for both class-level and method-level type parameters. The hasJSpecifyAnnotationDeep method is designed to check type uses (return types, parameters, etc.) and does not need a TYPEVAR case because type variable declaration bounds are already handled separately.
Applied to files:
jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.javajdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/CreateNestedAnnotationInfoVisitor.javajdk-javac-plugin/src/test/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializerTest.java
📚 Learning: 2025-08-28T04:54:20.953Z
Learnt from: msridhar
Repo: uber/NullAway PR: 1248
File: nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java:847-857
Timestamp: 2025-08-28T04:54:20.953Z
Learning: In NullAway's GenericsChecks.java, NewClassTree support for explicit type argument substitution requires more extensive changes beyond just modifying the conditional in compareGenericTypeParameterNullabilityForCall. The maintainers prefer to handle NewClassTree support in a separate follow-up rather than expanding the scope of PRs focused on specific issues like super constructor calls.
Applied to files:
jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.javajdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/CreateNestedAnnotationInfoVisitor.javajdk-javac-plugin/src/test/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializerTest.java
📚 Learning: 2025-08-14T18:50:06.159Z
Learnt from: msridhar
Repo: uber/NullAway PR: 1245
File: guava-recent-unit-tests/src/test/java/com/uber/nullaway/guava/NullAwayGuavaParametricNullnessTests.java:101-102
Timestamp: 2025-08-14T18:50:06.159Z
Learning: In NullAway JSpecify tests, when JDK version requirements exist due to bytecode annotation reading capabilities, prefer failing tests over skipping them on unsupported versions to ensure CI catches regressions and enforces proper JDK version usage for developers.
Applied to files:
jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.javajdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/CreateNestedAnnotationInfoVisitor.javajdk-javac-plugin/src/test/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializerTest.java
📚 Learning: 2025-09-24T23:14:37.777Z
Learnt from: haewiful
Repo: uber/NullAway PR: 1243
File: jdk-annotations/astubx-generator-cli/src/main/java/com/uber/nullaway/jdkannotations/AstubxGeneratorCLI.java:97-106
Timestamp: 2025-09-24T23:14:37.777Z
Learning: In the jdk-javac-plugin NullnessAnnotationSerializer, the plugin only creates one JSON file per compilation, so processing only the first JSON file is intentional behavior rather than a bug.
Applied to files:
jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.javajdk-javac-plugin/src/test/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializerTest.java
📚 Learning: 2025-09-03T19:40:11.118Z
Learnt from: msridhar
Repo: uber/NullAway PR: 1276
File: nullaway/src/main/java/com/uber/nullaway/generics/TypeMetadataBuilder.java:235-241
Timestamp: 2025-09-03T19:40:11.118Z
Learning: In JDK 21+, ListBuffer can be passed directly to TypeMetadata.Annotations constructor without needing conversion to List via toList() method. The APIs were updated to make this compatible.
Applied to files:
jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.java
📚 Learning: 2025-12-31T23:59:20.009Z
Learnt from: msridhar
Repo: uber/NullAway PR: 1421
File: nullaway/src/main/java/com/uber/nullaway/NullAway.java:339-340
Timestamp: 2025-12-31T23:59:20.009Z
Learning: In Error Prone's ASTHelpers class, the getSymbol(MethodInvocationTree) overload is guaranteed to return a non-null Symbol.MethodSymbol. This differs from other overloads of getSymbol() that may return null. Therefore, no null check is required after calling ASTHelpers.getSymbol() with a MethodInvocationTree parameter.
Applied to files:
jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.java
📚 Learning: 2025-08-29T18:41:43.584Z
Learnt from: msridhar
Repo: uber/NullAway PR: 1259
File: jdk-recent-unit-tests/src/test/java/com/uber/nullaway/jdk17/SwitchTests.java:318-321
Timestamp: 2025-08-29T18:41:43.584Z
Learning: Classes annotated with NullMarked are analyzed by NullAway even if they are not in packages specified by the AnnotatedPackages configuration. The NullMarked annotation guarantees NullAway analysis.
Applied to files:
jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/CreateNestedAnnotationInfoVisitor.javajdk-javac-plugin/src/test/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializerTest.java
🔇 Additional comments (9)
jdk-javac-plugin/src/test/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializerTest.java (2)
119-120: LGTM: MethodInfo expectations correctly updated with empty nestedAnnotationsList maps.The test expectations have been properly updated to include
Map.of()for the newnestedAnnotationsListfield inMethodInfo. The non-empty map cases (lines 354-364, 385-392) correctly model nested annotations with appropriateTypePathEntrypaths.Also applies to: 154-155, 213-214, 250-251, 294-295, 301-302, 308-309, 347-348, 354-364, 370-371, 377-378, 384-392
395-490: LGTM: Comprehensive test coverage for nested annotations.The
nestedAnnotationsForMethods()test effectively validates nested annotation extraction across multiple scenarios:
- Type argument annotations (
Class<@NonNull E>)- Return type nested annotations (
List<@Nullable String>)- Parameter type nested annotations with arrays (
@Nullable String[],List<@NonNull String>[])- Wildcard upper bound annotations (
? extends@nullableString)The
TypePathEntryconstructions correctly model the type paths (e.g.,ARRAY_ELEMENTwith index-1,WILDCARD_BOUNDwith index0for extends bound).jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/CreateNestedAnnotationInfoVisitor.java (3)
49-56: LGTM: Array type handling.The visitor correctly pushes an
ARRAY_ELEMENTpath entry, checks for annotations on the element type, recursively visits the element type, and pops the path entry.
58-79: LGTM: Wildcard type handling.The visitor correctly handles both upper bounds (
? extends T, index 0) and lower bounds (? super T, index 1) with separate path entries and recursive visits. The scoped handling addresses the stale variable issue from past reviews.
90-96: LGTM: Clean helper method for annotation collection.The extracted
addNestedAnnotationInfomethod consolidates the annotation-check-and-add logic, improving readability and reducing duplication.jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.java (4)
53-59: LGTM: MethodInfo record extended with nested annotations.The
nestedAnnotationsListfield is appropriately typed asMap<Integer, Set<NestedAnnotationInfo>>, using-1for return type and parameter indices for parameters. This aligns with standard JVM type annotation indexing conventions.
160-172: LGTM: Return type nested annotation collection.The nested annotation collection for return types is correctly gated by
hasJSpecifyAnnotationDeep()and uses key-1in the map. The visitor pattern properly extracts nested annotations.
177-189: LGTM: Parameter nested annotation collection fixed.The parameter scanning loop now runs unconditionally, correctly collecting nested annotations for all parameters regardless of whether the return type has annotations. This addresses the bug identified in previous reviews.
197-204: LGTM: MethodInfo construction includes nested annotations map.The
nestedAnnotationsMapis correctly passed to theMethodInforecord constructor, ensuring nested annotation data is serialized to JSON.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Comment on lines
+86
to
+88
| public Set<NestedAnnotationInfo> getNestedAnnotationInfoSet() { | ||
| return nestedAnnotationInfoSet; | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider returning an unmodifiable view of the set.
getNestedAnnotationInfoSet() returns the internal mutable HashSet, allowing callers to modify the visitor's state. Consider returning Collections.unmodifiableSet(nestedAnnotationInfoSet) or Set.copyOf(nestedAnnotationInfoSet).
🤖 Prompt for AI Agents
In
`@jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/CreateNestedAnnotationInfoVisitor.java`
around lines 86 - 88, getNestedAnnotationInfoSet currently exposes the internal
mutable HashSet allowing external mutation; change the getter in
CreateNestedAnnotationInfoVisitor to return an unmodifiable view (e.g.,
Collections.unmodifiableSet(nestedAnnotationInfoSet) or
Set.copyOf(nestedAnnotationInfoSet)) so callers cannot modify the visitor's
internal state, keeping the field type and name (nestedAnnotationInfoSet,
getNestedAnnotationInfoSet) unchanged.
msridhar
reviewed
Jan 14, 2026
...vac-plugin/src/test/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializerTest.java
Show resolved
Hide resolved
msridhar
reviewed
Jan 15, 2026
...ac-plugin/src/main/java/com/uber/nullaway/javacplugin/CreateNestedAnnotationInfoVisitor.java
Outdated
Show resolved
Hide resolved
...ac-plugin/src/main/java/com/uber/nullaway/javacplugin/CreateNestedAnnotationInfoVisitor.java
Outdated
Show resolved
Hide resolved
jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.java
Outdated
Show resolved
Hide resolved
jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.java
Outdated
Show resolved
Hide resolved
…reateNestedAnnotationInfoVisitor.java Co-authored-by: Manu Sridharan <[email protected]>
…reateNestedAnnotationInfoVisitor.java Co-authored-by: Manu Sridharan <[email protected]>
…ullnessAnnotationSerializer.java Co-authored-by: Manu Sridharan <[email protected]>
…ullnessAnnotationSerializer.java Co-authored-by: Manu Sridharan <[email protected]>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.java (1)
197-204: Consider making the nested annotations map immutable.The
nestedAnnotationsMapis passed directly toMethodInfo. While the map isn't modified after construction in the current code, wrapping withMap.copyOf()would provide defensive immutability, especially since the data is serialized to JSON and consumed externally.♻️ Suggested defensive copy
MethodInfo methodInfo = new MethodInfo( returnType, mSym.toString(), hasNullMarked, hasNullUnmarked, methodTypeParams, - nestedAnnotationsMap); + Map.copyOf(nestedAnnotationsMap));
♻️ Duplicate comments (1)
jdk-javac-plugin/src/test/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializerTest.java (1)
513-532: ImmutableList adapter implementation is functional.The adapter correctly deserializes parameterized
ImmutableListtypes by extracting the component type and delegating to Gson's context. The exception for raw types is acceptable in test code where parameterized types are always expected.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.javajdk-javac-plugin/src/test/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializerTest.java
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: msridhar
Repo: uber/NullAway PR: 1248
File: nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java:847-857
Timestamp: 2025-08-28T04:54:20.953Z
Learning: In NullAway's GenericsChecks.java, NewClassTree support for explicit type argument substitution requires more extensive changes beyond just modifying the conditional in compareGenericTypeParameterNullabilityForCall. The maintainers prefer to handle NewClassTree support in a separate follow-up rather than expanding the scope of PRs focused on specific issues like super constructor calls.
Learnt from: haewiful
Repo: uber/NullAway PR: 1243
File: jdk-annotations/astubx-generator-cli/src/main/java/com/uber/nullaway/jdkannotations/AstubxGeneratorCLI.java:97-106
Timestamp: 2025-09-24T23:14:37.777Z
Learning: In the jdk-javac-plugin NullnessAnnotationSerializer, the plugin only creates one JSON file per compilation, so processing only the first JSON file is intentional behavior rather than a bug.
Learnt from: msridhar
Repo: uber/NullAway PR: 1316
File: jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.java:261-293
Timestamp: 2025-10-29T23:56:18.236Z
Learning: In NullAway's jdk-javac-plugin NullnessAnnotationSerializer, type variable bounds with annotations (e.g., `T extends Nullable Object`) are checked at their declaration sites by the typeParamHasAnnotation method for both class-level and method-level type parameters. The hasJSpecifyAnnotationDeep method is designed to check type uses (return types, parameters, etc.) and does not need a TYPEVAR case because type variable declaration bounds are already handled separately.
Learnt from: msridhar
Repo: uber/NullAway PR: 1245
File: guava-recent-unit-tests/src/test/java/com/uber/nullaway/guava/NullAwayGuavaParametricNullnessTests.java:101-102
Timestamp: 2025-08-14T18:50:06.159Z
Learning: In NullAway JSpecify tests, when JDK version requirements exist due to bytecode annotation reading capabilities, prefer failing tests over skipping them on unsupported versions to ensure CI catches regressions and enforces proper JDK version usage for developers.
📚 Learning: 2025-08-14T18:50:06.159Z
Learnt from: msridhar
Repo: uber/NullAway PR: 1245
File: guava-recent-unit-tests/src/test/java/com/uber/nullaway/guava/NullAwayGuavaParametricNullnessTests.java:101-102
Timestamp: 2025-08-14T18:50:06.159Z
Learning: In NullAway JSpecify tests, when JDK version requirements exist due to bytecode annotation reading capabilities, prefer failing tests over skipping them on unsupported versions to ensure CI catches regressions and enforces proper JDK version usage for developers.
Applied to files:
jdk-javac-plugin/src/test/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializerTest.javajdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.java
📚 Learning: 2025-09-24T23:14:37.777Z
Learnt from: haewiful
Repo: uber/NullAway PR: 1243
File: jdk-annotations/astubx-generator-cli/src/main/java/com/uber/nullaway/jdkannotations/AstubxGeneratorCLI.java:97-106
Timestamp: 2025-09-24T23:14:37.777Z
Learning: In the jdk-javac-plugin NullnessAnnotationSerializer, the plugin only creates one JSON file per compilation, so processing only the first JSON file is intentional behavior rather than a bug.
Applied to files:
jdk-javac-plugin/src/test/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializerTest.javajdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.java
📚 Learning: 2025-08-28T04:54:20.953Z
Learnt from: msridhar
Repo: uber/NullAway PR: 1248
File: nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java:847-857
Timestamp: 2025-08-28T04:54:20.953Z
Learning: In NullAway's GenericsChecks.java, NewClassTree support for explicit type argument substitution requires more extensive changes beyond just modifying the conditional in compareGenericTypeParameterNullabilityForCall. The maintainers prefer to handle NewClassTree support in a separate follow-up rather than expanding the scope of PRs focused on specific issues like super constructor calls.
Applied to files:
jdk-javac-plugin/src/test/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializerTest.javajdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.java
📚 Learning: 2025-10-29T23:56:18.236Z
Learnt from: msridhar
Repo: uber/NullAway PR: 1316
File: jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.java:261-293
Timestamp: 2025-10-29T23:56:18.236Z
Learning: In NullAway's jdk-javac-plugin NullnessAnnotationSerializer, type variable bounds with annotations (e.g., `T extends Nullable Object`) are checked at their declaration sites by the typeParamHasAnnotation method for both class-level and method-level type parameters. The hasJSpecifyAnnotationDeep method is designed to check type uses (return types, parameters, etc.) and does not need a TYPEVAR case because type variable declaration bounds are already handled separately.
Applied to files:
jdk-javac-plugin/src/test/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializerTest.javajdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.java
📚 Learning: 2025-08-29T18:41:43.584Z
Learnt from: msridhar
Repo: uber/NullAway PR: 1259
File: jdk-recent-unit-tests/src/test/java/com/uber/nullaway/jdk17/SwitchTests.java:318-321
Timestamp: 2025-08-29T18:41:43.584Z
Learning: Classes annotated with NullMarked are analyzed by NullAway even if they are not in packages specified by the AnnotatedPackages configuration. The NullMarked annotation guarantees NullAway analysis.
Applied to files:
jdk-javac-plugin/src/test/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializerTest.java
📚 Learning: 2025-09-03T19:40:11.118Z
Learnt from: msridhar
Repo: uber/NullAway PR: 1276
File: nullaway/src/main/java/com/uber/nullaway/generics/TypeMetadataBuilder.java:235-241
Timestamp: 2025-09-03T19:40:11.118Z
Learning: In JDK 21+, ListBuffer can be passed directly to TypeMetadata.Annotations constructor without needing conversion to List via toList() method. The APIs were updated to make this compatible.
Applied to files:
jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.java
📚 Learning: 2025-12-31T23:59:20.009Z
Learnt from: msridhar
Repo: uber/NullAway PR: 1421
File: nullaway/src/main/java/com/uber/nullaway/NullAway.java:339-340
Timestamp: 2025-12-31T23:59:20.009Z
Learning: In Error Prone's ASTHelpers class, the getSymbol(MethodInvocationTree) overload is guaranteed to return a non-null Symbol.MethodSymbol. This differs from other overloads of getSymbol() that may return null. Therefore, no null check is required after calling ASTHelpers.getSymbol() with a MethodInvocationTree parameter.
Applied to files:
jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.java
🔇 Additional comments (8)
jdk-javac-plugin/src/test/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializerTest.java (4)
6-30: LGTM: Import additions.The new imports support the ImmutableList adapter (JsonDeserializer, JsonArray, JsonElement, ParameterizedType, Type), nested annotation types (NestedAnnotationInfo, TypePathEntry, Annotation), and Set for the nested annotation map values.
119-120: LGTM: Updated test expectations with empty nested annotation maps.The
Map.of()additions align with the updatedMethodInforecord signature that now includesnestedAnnotationsList.
395-507: Comprehensive nested annotation test coverage.This test method thoroughly covers:
- Parameter nested annotations (
@NonNull Ein type argument)- Return type nested annotations (
List<@Nullable String>)- Array element annotations (
@Nullable String [])- Arrays of parameterized types (
List<@NonNull String>[])- Wildcard upper bounds (
? extends@nullableString)- Nested type arguments without array elements (
List<List<String@nullable[]>>)The test cases address the scenarios requested in past review comments, including the
List<List<String@nullable[]>>case that should have two type arguments but no array elements in the type path.
355-364: No action required—the wildcard bound index convention is correct.The test at line 364 correctly uses
WILDCARD_BOUNDindex1for the super bound (? super@nullableInteger). This aligns with the documented convention: index0for upper bounds (? extends) and index1for lower bounds (? super), as specified in the TypePathEntry javadoc and implemented consistently in CreateNestedAnnotationInfoVisitor.jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.java (4)
28-28: LGTM: Import addition for Set.Required for the
Map<Integer, Set<NestedAnnotationInfo>>type used inMethodInfo.
53-59: LGTM: MethodInfo record updated with nested annotation tracking.The
nestedAnnotationsListfield captures nested annotations for both return type (key-1) and parameters (key = parameter index). TheMap<Integer, Set<NestedAnnotationInfo>>structure efficiently supports multiple nested annotations per position.
158-172: LGTM: Return type nested annotation collection.The implementation correctly:
- Creates a fresh visitor per return type
- Guards collection with
hasJSpecifyAnnotationDeepcheck- Only adds to the map if nested annotations exist
The visitor pattern is applied consistently with the parameter handling below.
177-189: LGTM: Parameter nested annotation collection.The implementation correctly iterates over all parameters unconditionally, ensuring nested annotations are collected regardless of whether the return type already set
methodHasAnnotations. This addresses the bug flagged in previous reviews where parameters were skipped when the return type had annotations.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This pull requests updates the jdk-javac-plugin to parse the nested annotations in external libraries and creates JSON files according to it. For the nested annotation information, it uses
NestedAnnotationInfoclass. It is the first step from #1410.The added unit test for this is
nestedAnnotationsForMethods().Summary by CodeRabbit
New Features
Serialization / Compatibility
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.